Skip to content

Conversation

Jiya873
Copy link

@Jiya873 Jiya873 commented May 20, 2025

This pull request addresses issue #777 where calling cnv_discordant_read_calls with absent data was causing an IndexError instead of the expected ValueError. The changes include:

  • Refactoring cnv_discordant_read_calls:
    The empty-data check is now moved outside the inner loop that iterates over the sample sets. If no dataset is found for a contig (or across all contigs), a ValueError is raised with an informative message.

  • Updated Tests:
    The relevant tests in test_cnv_data.py have been leveraged to confirm that the expected ValueError is raised in cases where data is absent (e.g., for non-existent sample sets or contigs).

These changes ensure that the API behaves more predictably and makes debugging easier when data is missing.

@jonbrenas
Copy link
Collaborator

jonbrenas commented May 20, 2025

This looks good, thank you @Jiya873. I am going to shadow this PR to enable the checks.

If you updated test_cnv_data.py, you forgot to push your changed as they are not showing currently.

Copy link

codecov bot commented May 20, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.07%. Comparing base (0197957) to head (f56c6d8).
Report is 50 commits behind head on master.

Files with missing lines Patch % Lines
malariagen_data/anoph/cnv_data.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #781      +/-   ##
==========================================
- Coverage   96.13%   96.07%   -0.06%     
==========================================
  Files          47       47              
  Lines        4683     4689       +6     
==========================================
+ Hits         4502     4505       +3     
- Misses        181      184       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Jiya873
Copy link
Author

Jiya873 commented May 21, 2025

Hi @jonbrenas, I had indeed forgotten to push the updated test_cnv_data.py earlier. I've just pushed the changes now. Please let me know if everything looks good or if further updates are needed.

@jonbrenas
Copy link
Collaborator

Thank you @Jiya873. It looks like the tests don't like your monkeypatching. I will try to look at what seems to be the problem ...if you don't beat me to it.

@leehart
Copy link
Collaborator

leehart commented Aug 8, 2025

Not sure what state this PR is in?

@leehart leehart requested a review from jonbrenas August 8, 2025 08:41
@jonbrenas
Copy link
Collaborator

I think it has been abandoned.

@leehart
Copy link
Collaborator

leehart commented Aug 8, 2025

Thanks @jonbrenas . If there's no response after a short time (say, September) then I guess we'll close this. This PR can always be resurrected, if needs be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants